drpcmux: add support for server interceptors #10
Conversation
cthumuluru-crdb
left a comment
cthumuluru-crdb
left a comment
There was a problem hiding this comment.
I left a couple of minor comments. Otherwise the changes look good to me.
|
|
||
| type UnaryHandler func(ctx context.Context, in interface{}) (out interface{}, err error) | ||
|
|
||
| type UnaryServerInterceptor func( |
There was a problem hiding this comment.
Add comments and contract for the interceptors.
| if currIdx == len(interceptors) { | ||
| return handler | ||
| } | ||
| return func(ctx context.Context, in interface{}) (out interface{}, err error) { |
There was a problem hiding this comment.
How about extracting this into asUnaryHandler(i UnaryInterceptor) UnaryHandler function? I find it more readable and easy to understand.
There was a problem hiding this comment.
I think it's a bit more complicated. We're not just adapting the interceptor as a handler; we're also capturing arguments from getChainedUnaryHandler. This means if a method is declared as asUnaryHandler, it will still accept those arguments, making the cleanup pointless.
There was a problem hiding this comment.
Yeah, you are right. I would suggest adding a comment on what that code is meant to do for future reference.
| type StreamHandler func(ctx context.Context, in drpc.Stream) (out interface{}, err error) | ||
|
|
||
| type StreamServerInterceptor func( | ||
| ctx context.Context, stream drpc.Stream, rpc string, handler StreamHandler) (out interface{}, err error) |
There was a problem hiding this comment.
Like we discussed before, rpc string can be replaced with StreamServerInfo similar to gRPC. Since stream interceptors apply to all forms of stream RPCs (unary request + stream response, stream request + unary response, stream request + stream response), this metadata will be useful for writing an interceptor that only applies to one form of stream interceptor.
There was a problem hiding this comment.
I'm in favor of adding this, but I'm waiting to come across any use case where we need more than an RPC string.
There was a problem hiding this comment.
I'm not necessarily suggesting to block on this but IMO interfaces should also help with future needs. I'm fine with the current approach and revisit it if needed.
shubhamdhama
force-pushed
the
drpc/server-interceptors-fx-changes
branch
from
3c6288c to
5d8e4dc
Compare
| if currIdx == len(interceptors) { | ||
| return handler | ||
| } | ||
| return func(ctx context.Context, in interface{}) (out interface{}, err error) { |
There was a problem hiding this comment.
Yeah, you are right. I would suggest adding a comment on what that code is meant to do for future reference.
| type StreamHandler func(ctx context.Context, in drpc.Stream) (out interface{}, err error) | ||
|
|
||
| type StreamServerInterceptor func( | ||
| ctx context.Context, stream drpc.Stream, rpc string, handler StreamHandler) (out interface{}, err error) |
There was a problem hiding this comment.
I'm not necessarily suggesting to block on this but IMO interfaces should also help with future needs. I'm fine with the current approach and revisit it if needed.
This change introduces support for server-side interceptors in DRPC. For a deeper understanding of interceptors, refer to the gRPC guide: https://grpc.io/docs/guides/interceptors lthough this functionality logically belongs in the `drpcserver` package, implementing it within `drpcmux` offers a more practical approach, avoiding a substantial refactor.
shubhamdhama
force-pushed
the
drpc/server-interceptors-fx-changes
branch
from
5d8e4dc to
95e1d1c
Compare
In cockroachdb/drpc#10, we added the DRPC framework level support for interceptors. This PR ports the metrics interceptor to DRPC server. Epic: CRDB-49359 Release note: None
2 participants
This change introduces support for server-side interceptors in DRPC. For a deeper understanding of interceptors, refer to the gRPC guide: https://grpc.io/docs/guides/interceptors
Although this functionality logically belongs in the
drpcserverpackage, implementing it withindrpcmuxoffers a more practical approach, avoiding a substantial refactor.Fixes: cockroachdb/cockroach#147622